Skip to content

fix: stop rewriting user SQL in query execution layer (#956)#960

Merged
datlechin merged 14 commits intomainfrom
fix/956-stop-rewriting-user-sql
May 1, 2026
Merged

fix: stop rewriting user SQL in query execution layer (#956)#960
datlechin merged 14 commits intomainfrom
fix/956-stop-rewriting-user-sql

Conversation

@datlechin
Copy link
Copy Markdown
Member

Summary

  • Closes sqlite3 does not apply limit? #956. SQLite (and several other drivers) silently stripped the user's LIMIT clause and substituted the app's row cap, so SELECT ... LIMIT 10 returned 10,000 rows.
  • Removes the fetchFirstPage / fetchNextPage / fetchRows / fetchRowCount family from the driver protocol and replaces it with executeUserQuery(query:rowCap:parameters:). User SQL passes to the database unchanged; the row cap is enforced at the result level instead of by query rewrite.
  • Replaces the broken Load More button on user-query tabs with a Showing N rows (truncated) [Fetch All] banner. Table-tab pagination is unchanged.

What changed

  • Driver protocol (PluginDatabaseDriver, DatabaseDriver, PluginDriverAdapter): six paged methods removed, executeUserQuery added with a Strategy B default (run query, slice client-side if rows exceed cap, set isTruncated). PluginKit ABI bumped from 8 to 9.
  • Plugins: stripLimitOffset regex removed from SQLite, MySQL, PostgreSQL, Redshift, ClickHouse, DuckDB, LibSQL, CloudflareD1. SQLite override uses Strategy A (collect from streamRows, break at cap). All other SQL plugins inherit Strategy B from the protocol default. MSSQL/Oracle no longer inject ORDER BY 1 / ORDER BY (SELECT NULL) into user SQL. All plugins bumped to ABI 9 in Info.plist.
  • MCP: MCPConnectionBridge.executeQuery now calls executeUserQuery and reads isTruncated from the driver instead of computing it from a +1 probe. The timeout race and cancelQuery wiring are unchanged.
  • App layer: MainContentCoordinator+QueryHelpers and +LoadMore updated; loadMoreRows() deleted; fetchAllRows() routes through executeUserQuery(rowCap: nil). pagination.hasMoreRows semantics narrow to "result was truncated".
  • Settings: enforceQueryResultLimit renamed to truncateQueryResults, queryResultLimit to queryResultRowCap. No fallback decode. Custom values revert to default on first launch.
  • UI: Status bar banner replaces the Load More + Fetch All button row on query tabs.
  • Docs: docs/features/data-grid.mdx Progressive Loading section rewritten as Result Truncation. Stale "sorting is local to the loaded page" line corrected.
  • Tests: Regression coverage in TableProTests/Core/Database/... for SQLite covering user LIMIT, OFFSET, CTE, UNION, subquery, truncation flag, and Fetch All.

Test plan

Mandatory regression on SQLite (use the issue reporter's database or any 1000+ row table):

  • SELECT * FROM t LIMIT 10 returns exactly 10 rows, no truncation banner
  • SELECT * FROM t LIMIT 5 OFFSET 50 returns 5 rows starting at row 51
  • WITH cte AS (SELECT * FROM t LIMIT 3) SELECT * FROM cte returns 3 rows
  • SELECT * FROM t on a 50k-row table shows "Showing 10,000 rows (truncated)" + Fetch All
  • Click Fetch All in the truncated case loads all rows

Cross-plugin smoke (whichever you have access to): same LIMIT 10 query against MySQL, PostgreSQL, Redshift, MSSQL, Oracle, ClickHouse, DuckDB, LibSQL, CloudflareD1, BigQuery.

Settings:

  • On first launch, custom queryResultLimit value is gone (revert to 10,000 default; per CHANGELOG)
  • Toggle "Truncate query results" off, run no-LIMIT query → no banner, all rows return
  • Set Row cap to 5,000 → banner shows "Showing 5,000 rows (truncated)"

Non-regression:

  • Table-tab pagination (page nav, page size, sort, filter, FK navigation) works as before
  • Streaming export of a query with a user LIMIT respects the LIMIT
  • Cancel (Cmd+.) interrupts a long-running query

MCP path:

  • Raycast / external client SELECT * FROM huge_table LIMIT 5 returns 5 rows, is_truncated: false
  • Same query with low maxRows (e.g. 10) returns 10 rows, is_truncated: true
  • Query with trailing semicolons (SELECT 1;;;) executes cleanly

External plugins:

  • Separately distributed plugins (Oracle, DuckDB, MSSQL, MongoDB, BigQuery, LibSQL, Cassandra, Etcd, CloudflareD1, DynamoDB) need re-release at ABI 9. Plugin Manager should mark old versions as outdated.

Manual screenshot follow-up: re-capture images/progressive-loading.png and the dark variant after build (UI changed from button row to banner row).

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c4624cf17a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

return nil
}
return (false, 0)
return dataGridSettings.validatedQueryResultRowCap
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Treat zero row cap as unlimited before executing user query

resolveRowCap now returns validatedQueryResultRowCap directly, but that setting explicitly uses 0 to mean “unlimited”. Passing 0 into executeUserQuery is interpreted as a real cap, so any non-empty result gets truncated to zero rows (prefix(0)) and marked truncated. This breaks query execution whenever users have row cap set to 0 with truncation enabled (including migrated/manual settings), returning empty grids instead of full results.

Useful? React with 👍 / 👎.

@datlechin datlechin merged commit 3f7a886 into main May 1, 2026
2 checks passed
@datlechin datlechin deleted the fix/956-stop-rewriting-user-sql branch May 1, 2026 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

sqlite3 does not apply limit?

1 participant